fix(feature_flags): handle expected falsy values in conditions #2052

ran-isenberg · 2023-03-26T14:08:07Z

Issue number: #2051

Summary

Rule Conditions allow users to evaluate context attributes against values and actions defined in Rule Conditions.
The condition can easily be pivoted on a certain value in the context evaluating to falsy values
The values attribute should be able to accept any value except null
The condition should also honor any value for context key variable.

Changes

Added tests
changed the validation from if true to if none
changed error message to none instead of empty

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Meet tenets criteria
I have performed a self-review of this change
Changes have been tested
Changes are documented
PR title follows conventional commit semantics

Is this a breaking change?

RFC issue number:

Checklist:

Migration process documented
Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

…nnot have Falsy values

heitorlessa

looks great, thanks for the additional tests!

Only a minor semantic correction, pushing from GitHub UI so we can make a patch release.

aws_lambda_powertools/utilities/feature_flags/schema.py

tests/functional/feature_flags/test_schema_validation.py

heitorlessa · 2023-03-27T12:31:25Z

hmmm, GitHub review is messing up indentation and isn't signing off commit to push the change. Can't push to your fork either - here's the patch with the changes whenever you can.

From bd8a6215e4815cc729d0c695d42a14144bc56ee7 Mon Sep 17 00:00:00 2001
From: heitorlessa <[email protected]>
Date: Mon, 27 Mar 2023 14:26:08 +0200
Subject: [PATCH] chore: use null instead of none in err msg

---
 aws_lambda_powertools/utilities/feature_flags/schema.py  | 2 +-
 tests/functional/feature_flags/test_schema_validation.py | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py
index df999a51..08f2ee13 100644
--- a/aws_lambda_powertools/utilities/feature_flags/schema.py
+++ b/aws_lambda_powertools/utilities/feature_flags/schema.py
@@ -327,7 +327,7 @@ class ConditionsValidator(BaseValidator):
     def validate_condition_value(condition: Dict[str, Any], rule_name: str):
         value = condition.get(CONDITION_VALUE)
         if value is None:
-            raise SchemaValidationError(f"'value' key must not be none, rule={rule_name}")
+            raise SchemaValidationError(f"'value' key must not be null, rule={rule_name}")
         action = condition.get(CONDITION_ACTION, "")
 
         # time actions need to be parsed to make sure date and time format is valid and timezone is recognized
diff --git a/tests/functional/feature_flags/test_schema_validation.py b/tests/functional/feature_flags/test_schema_validation.py
index 8441b944..8d3b97ad 100644
--- a/tests/functional/feature_flags/test_schema_validation.py
+++ b/tests/functional/feature_flags/test_schema_validation.py
@@ -301,7 +301,7 @@ def test_validate_condition_missing_condition_value():
     }
 
     # WHEN calling validate_condition
-    with pytest.raises(SchemaValidationError, match="'value' key must not be none"):
+    with pytest.raises(SchemaValidationError, match="'value' key must not be null"):
         ConditionsValidator.validate_condition_value(condition=condition, rule_name="dummy")
 
 
@@ -314,7 +314,7 @@ def test_validate_condition_none_condition_value():
     }
 
     # WHEN calling validate_condition
-    with pytest.raises(SchemaValidationError, match="'value' key must not be none"):
+    with pytest.raises(SchemaValidationError, match="'value' key must not be null"):
         ConditionsValidator.validate_condition_value(condition=condition, rule_name="dummy")
 
 
-- 
2.37.1 (Apple Git-137.1)

heitorlessa · 2023-03-27T12:31:50Z

hmmm, GitHub review is messing up indentation and isn't signing off commit to push the change. Can't push to your fork either - here's the git patch with the changes whenever you can.

From bd8a6215e4815cc729d0c695d42a14144bc56ee7 Mon Sep 17 00:00:00 2001
From: heitorlessa <[email protected]>
Date: Mon, 27 Mar 2023 14:26:08 +0200
Subject: [PATCH] chore: use null instead of none in err msg

---
 aws_lambda_powertools/utilities/feature_flags/schema.py  | 2 +-
 tests/functional/feature_flags/test_schema_validation.py | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py
index df999a51..08f2ee13 100644
--- a/aws_lambda_powertools/utilities/feature_flags/schema.py
+++ b/aws_lambda_powertools/utilities/feature_flags/schema.py
@@ -327,7 +327,7 @@ class ConditionsValidator(BaseValidator):
     def validate_condition_value(condition: Dict[str, Any], rule_name: str):
         value = condition.get(CONDITION_VALUE)
         if value is None:
-            raise SchemaValidationError(f"'value' key must not be none, rule={rule_name}")
+            raise SchemaValidationError(f"'value' key must not be null, rule={rule_name}")
         action = condition.get(CONDITION_ACTION, "")
 
         # time actions need to be parsed to make sure date and time format is valid and timezone is recognized
diff --git a/tests/functional/feature_flags/test_schema_validation.py b/tests/functional/feature_flags/test_schema_validation.py
index 8441b944..8d3b97ad 100644
--- a/tests/functional/feature_flags/test_schema_validation.py
+++ b/tests/functional/feature_flags/test_schema_validation.py
@@ -301,7 +301,7 @@ def test_validate_condition_missing_condition_value():
     }
 
     # WHEN calling validate_condition
-    with pytest.raises(SchemaValidationError, match="'value' key must not be none"):
+    with pytest.raises(SchemaValidationError, match="'value' key must not be null"):
         ConditionsValidator.validate_condition_value(condition=condition, rule_name="dummy")
 
 
@@ -314,7 +314,7 @@ def test_validate_condition_none_condition_value():
     }
 
     # WHEN calling validate_condition
-    with pytest.raises(SchemaValidationError, match="'value' key must not be none"):
+    with pytest.raises(SchemaValidationError, match="'value' key must not be null"):
         ConditionsValidator.validate_condition_value(condition=condition, rule_name="dummy")
 
 
-- 
2.37.1 (Apple Git-137.1)

ajwad-shaikh

@heitorlessa @ran-isenberg

There are a few tests that need fix - the expected value is the same as non-expected value which means that these tests will never fail

test_flags_not_equal_match
test_flags_less_than_match
test_flags_less_than_or_equal_match_1
test_flags_less_than_or_equal_match_2
test_flags_greater_than_match
test_flags_greater_than_or_equal_match_1
test_flags_greater_than_or_equal_match_2

Apart from these the sample test case in the documentation also has the same flaw.

I'll be happy to raise a separate PR for this but since fixing one of these tests would have failed if did not have the flaw, I thought we can ship these together under common context. Let me know your thoughts

heitorlessa · 2023-03-27T16:49:11Z

@ajwad-shaikh had an idea now that GitHub seems to be back -- create a new PR (I'm merging this shortly), so we can make sure you're credited properly in your first contribution <3

Signed-off-by: Heitor Lessa <[email protected]>

heitorlessa · 2023-03-27T16:56:46Z

For future reference until we manage to create issues for these, two things I'd love to do/get help in Feature Flags:

Move most of the functional tests as unit tests (e.g., these Schema Validation are largely unit tests)
Investigate slowness in testing Feature Flags and refactor accordingly (e.g., we're always creating mapping_by_action lambda functions instead of making them constant) -- Feature Flags IIRC has the slowest test execution, py-spy should help produce a flame graph when running the test suite here

heitorlessa · 2023-03-27T16:59:01Z

Pinged you on Discord, I'll wait for the separate PR before we make a release. If you get busy, we'll create one by EOW and credit accordingly either way... but I'd really love to make sure you get your experience in making your first contribution ;)

Merging - thanks again @ran-isenberg !

fix: Feature Flag Rule Conditions Values and Context Key Variables ca…

3c3a9cb

…nnot have Falsy values

ran-isenberg requested a review from a team as a code owner March 26, 2023 14:08

ran-isenberg requested review from heitorlessa and removed request for a team March 26, 2023 14:08

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 26, 2023

boring-cyborg bot added the tests label Mar 26, 2023

ran-isenberg mentioned this pull request Mar 26, 2023

Bug: Feature Flag Rule Conditions Values and Context Key Variables cannot have Falsy values #2051

Closed

fix: Feature Flag Rule Conditions Values and Context Key Variables ca…

1006527

…nnot have Falsy values

github-actions bot added the bug Something isn't working label Mar 27, 2023

heitorlessa changed the title ~~fix: Feature Flag Rule Conditions Values and Context Key Variables ca…~~ fix(feature_flags): handle falsy values when they're meant to be falsy Mar 27, 2023

heitorlessa changed the title ~~fix(feature_flags): handle falsy values when they're meant to be falsy~~ fix(feature_flags): handle expected falsy values in conditions Mar 27, 2023

heitorlessa suggested changes Mar 27, 2023

View reviewed changes

aws_lambda_powertools/utilities/feature_flags/schema.py Outdated Show resolved Hide resolved

tests/functional/feature_flags/test_schema_validation.py Outdated Show resolved Hide resolved

tests/functional/feature_flags/test_schema_validation.py Outdated Show resolved Hide resolved

ajwad-shaikh reviewed Mar 27, 2023

View reviewed changes

heitorlessa added 3 commits March 27, 2023 18:52

chore: change to null

be1fdd3

Signed-off-by: Heitor Lessa <[email protected]>

chore: change to null

48e5294

Signed-off-by: Heitor Lessa <[email protected]>

chore: change to null

bf0c9a4

Signed-off-by: Heitor Lessa <[email protected]>

heitorlessa approved these changes Mar 27, 2023

View reviewed changes

heitorlessa merged commit ab9078c into aws-powertools:develop Mar 27, 2023

ran-isenberg deleted the fix-2051 branch March 27, 2023 17:12

ajwad-shaikh mentioned this pull request Mar 28, 2023

Bug: Feature Flags - Faulty Test Cases #2058

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(feature_flags): handle expected falsy values in conditions #2052

fix(feature_flags): handle expected falsy values in conditions #2052

ran-isenberg commented Mar 26, 2023 •

edited by heitorlessa

Loading

heitorlessa left a comment

heitorlessa commented Mar 27, 2023

heitorlessa commented Mar 27, 2023

ajwad-shaikh left a comment

heitorlessa commented Mar 27, 2023

heitorlessa commented Mar 27, 2023

heitorlessa commented Mar 27, 2023 •

edited

Loading

fix(feature_flags): handle expected falsy values in conditions #2052

fix(feature_flags): handle expected falsy values in conditions #2052

Conversation

ran-isenberg commented Mar 26, 2023 • edited by heitorlessa Loading

Summary

Changes

Checklist

Acknowledgment

heitorlessa left a comment

Choose a reason for hiding this comment

heitorlessa commented Mar 27, 2023

heitorlessa commented Mar 27, 2023

ajwad-shaikh left a comment

Choose a reason for hiding this comment

heitorlessa commented Mar 27, 2023

heitorlessa commented Mar 27, 2023

heitorlessa commented Mar 27, 2023 • edited Loading

ran-isenberg commented Mar 26, 2023 •

edited by heitorlessa

Loading

heitorlessa commented Mar 27, 2023 •

edited

Loading